Wrap ComfortViewFactorAngles by joseph-robertson · Pull Request #5650 · NatLabRockies/OpenStudio

joseph-robertson · 2026-09-08T17:33:06Z

Pull request overview

Closes Wrap ComfortViewFactorAngles #4412
This also:
- Enables using "Mean Radiant Temperature Calculation Type" other than EnclosureAveraged
- Moves (previously unused) "Surface Name/Angle Factor List Name" from People to People:Definition

Pull Request Author

Model API Changes / Additions
Any new or modified fields have been implemented in the EnergyPlus ForwardTranslator (and ReverseTranslator as appropriate)
Model API methods are tested (in src/model/test)
EnergyPlus ForwardTranslator Tests (in src/energyplus/Test)
If a new object or method, added a test in NREL/OpenStudio-resources: Wrap ComfortViewFactorAngles OpenStudio-resources#233
If needed, added VersionTranslation rules for the objects (src/osversion/VersionTranslator.cpp)
Verified that C# bindings built fine on Windows, partial classes used as needed, etc.
All new and existing tests passes
If methods have been deprecated, update rest of code to use the new methods

Labels:

If change to an IDD file, add the label IDDChange
If breaking existing API, add the label APIChange
If deemed ready, add label Pull Request - Ready for CI so that CI builds your PR

Review Checklist

This will not be exhaustively relevant to every PR.

Perform a Code Review on GitHub
Code Style, strip trailing whitespace, etc.
All related changes have been implemented: model changes, model tests, FT changes, FT tests, VersionTranslation, OS App
Labeling is ok
If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
If feature, test running new feature, try creative ways to break it
CI status: all green or justified

joseph-robertson · 2026-09-08T22:32:26Z

Um hang on, why is \object-list ZoneNames in the Name field? And Name needs to be \required-field, right? Also, like ZoneMRTCalculation, in addAngleFactor we need to check that "all surfaces are in the same enclosure"? And A1 jumps to A3 ... ?

ComfortViewFactorAngles,
       \memo Used to specify radiant view factors for thermal comfort calculations.
       \memo Note that the following angle factor fractions must sum up to 1.0
       \memo The number of surfaces can be expanded beyond 100, if necessary, by adding more
       \memo groups to the end of the list.
       \extensible:2
  A1 , \field Name
       \type alpha
       \reference AllHeatTranAngFacNames
       \object-list ZoneNames <--------------------------- ?
  A3 , \field Surface 1 Name
       \type object-list
       \object-list AllHeatTranSurfNames
       \begin-extensible
  N1 ; \field Angle Factor 1
       \type real
       \minimum 0.0
       \maximum 1.0

Copilot

🟡 Changes recommended

Version translation loses existing targets, and the wrapper omits valid target types while permitting inconsistent MRT state.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds model and EnergyPlus translation support for ComfortViewFactorAngles and related People MRT calculation modes.

Changes:

Adds the new model object and bindings.
Implements forward/reverse translation and tests.
Moves the MRT target field to PeopleDefinition with version translation.

File summaries

File	Description
`src/osversion/VersionTranslator.cpp`	Migrates People fields to 3.12.
`src/osversion/test/VersionTranslator_GTest.cpp`	Tests People migration.
`src/osversion/test/3_12_0/test_vt_People.rb`	Generates migration fixture.
`src/osversion/test/3_12_0/test_vt_People.osm`	Provides 3.11 fixture data.
`src/model/test/People_GTest.cpp`	Tests new PeopleDefinition API.
`src/model/test/ComfortViewFactorAngles_GTest.cpp`	Tests angle-factor behavior.
`src/model/PeopleDefinition.hpp`	Exposes MRT target API.
`src/model/PeopleDefinition.cpp`	Implements MRT target handling.
`src/model/PeopleDefinition_Impl.hpp`	Declares internal target methods.
`src/model/People_Impl.hpp`	Removes obsolete TODO.
`src/model/ModelGeometry.i`	Adds bindings.
`src/model/Model.cpp`	Registers model constructors.
`src/model/ConcreteModelObjects.hpp`	Includes new model types.
`src/model/ComfortViewFactorAngles.hpp`	Defines the public model API.
`src/model/ComfortViewFactorAngles.cpp`	Implements angle-factor logic.
`src/model/ComfortViewFactorAngles_Impl.hpp`	Defines internal implementation API.
`src/model/CMakeLists.txt`	Builds model sources and tests.
`src/energyplus/Test/People_GTest.cpp`	Tests People translation modes.
`src/energyplus/Test/ComfortViewFactorAngles_GTest.cpp`	Tests object translation.
`src/energyplus/ReverseTranslator/ReverseTranslatePeople.cpp`	Imports MRT targets.
`src/energyplus/ReverseTranslator/ReverseTranslateComfortViewFactorAngles.cpp`	Imports angle-factor lists.
`src/energyplus/ReverseTranslator.hpp`	Declares reverse translation.
`src/energyplus/ReverseTranslator.cpp`	Dispatches reverse translation.
`src/energyplus/ForwardTranslator/ForwardTranslatePeople.cpp`	Exports People MRT settings.
`src/energyplus/ForwardTranslator/ForwardTranslateComfortViewFactorAngles.cpp`	Exports angle-factor lists.
`src/energyplus/ForwardTranslator.hpp`	Declares forward translation.
`src/energyplus/ForwardTranslator.cpp`	Dispatches forward translation.
`src/energyplus/CMakeLists.txt`	Builds translator sources and tests.
`resources/model/OpenStudio.idd`	Adds and relocates model fields.
`resources/energyplus/ProposedEnergy+.idd`	Updates the EnergyPlus schema.

Review details

Suppressed comments (1)

src/model/PeopleDefinition.cpp:286

This rejects valid direct targets supported by EnergyPlus. Surface Name/Angle Factor List Name uses AllHeatTranAngFacNames, which includes fenestration and internal-mass objects as well as base surfaces and angle-factor lists (see resources/energyplus/ProposedEnergy+.idd:7191-7193,8022-8023,11971-11973). Consequently reverse translation silently loses a valid SubSurface or InternalMass target. Support the full target set rather than only Surface and ComfortViewFactorAngles.

      if (modelObject.optionalCast<Surface>()) {
        mrtType = "SurfaceWeighted";
      } else if (modelObject.optionalCast<ComfortViewFactorAngles>()) {
        mrtType = "AngleFactor";
      } else {
        LOG(Error, "Surface Name/Angle Factor List Name must reference a Surface or ComfortViewFactorAngles object.");

Files reviewed: 30/30 changed files
Comments generated: 4
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

joseph-robertson · 2026-09-09T16:31:54Z

+            } else if (i > 6) {
+              newObject.setString(i - 1, value.get());


joseph-robertson · 2026-09-09T16:43:56Z

+       \reference AllHeatTranAngFacNames
+  A3,  \field Surface 1 Name
+       \type object-list
+       \object-list SurfaceNames


joseph-robertson · 2026-09-09T16:31:37Z

+      if (result && istringEqual(mrtType, "EnclosureAveraged")) {
+        resetSurfaceNameAngleFactorListName();


jmarrec · 2026-09-09T20:18:59Z

Moves (previously unused) "Surface Name/Angle Factor List Name" from People to People:Definition

Why? I think the specific surfaces are instance specific rather than definition-level, don't you?

(Note: I didn't review the changes yet as I'm on a mobile device)

As far as the IDD inconsistencies, that should be fixed upstream on EnergyPlus with a quick PR

…ld indices

Require a name, declare the minimum field count, and document the enclosure requirement for referenced surfaces.

Validate surface space and thermal-zone assignments before adding angle factors, reject cross-zone lists, and align extensible-group update handling with ZoneMRTCalculation.

Revalidate every referenced surface's thermal zone before generating the EnergyPlus object and cover stale surface assignments in the forward-translator tests.

Reject non-finite angle factors and clear the MRT target when resetting the calculation type, with model regression coverage.

Move unambiguous 3.11 People MRT targets to PeopleDefinition during version translation, report shared-definition conflicts, and cover the migrated surface target.

Accept Surface, SubSurface, and InternalMass targets from AllHeatTranSurfNames, preserve same-zone validation, and cover model and EnergyPlus translation behavior.

github-actions · 2026-09-10T08:45:37Z

🧪 Test Results Dashboard

Summary

Metric	Value
Total Tests	0
Passed	0
Failed	0
Errors	0
Skipped	0
Success Rate	0.0%
Generated	2026-09-10 08:45:36 UTC

❌ Significant Test Failures

📊 Test Run Information

Run	XML File	Status
-	No XML files found	❌ Missing

joseph-robertson self-assigned this Sep 8, 2026

joseph-robertson added Pull Request - Ready for CI This pull request if finalized and is ready for continuous integration verification prior to merge. IDDChange labels Sep 8, 2026

joseph-robertson mentioned this pull request Sep 8, 2026

Wrap ZoneMRTCalculation NatLabRockies/OpenStudio-resources#232

Merged

42 tasks

joseph-robertson marked this pull request as ready for review September 8, 2026 22:21

joseph-robertson requested a review from jmarrec September 8, 2026 22:21

joseph-robertson requested a balanced review from Copilot September 9, 2026 15:30

Copilot started reviewing on behalf of joseph-robertson September 9, 2026 15:31 View session

Copilot AI reviewed Sep 9, 2026

View reviewed changes

Base automatically changed from v26.2.0-IOFreeze-ZoneMRTCalc to develop September 10, 2026 08:44

joseph-robertson added 18 commits September 10, 2026 10:45

Add ComfortViewFactorAngles model schema

848afbb

Add ComfortViewFactorAngles model object

6c9beaa

Translate ComfortViewFactorAngles for People

2fe1fb5

Import ComfortViewFactorAngles from EnergyPlus

f674777

Translate People MRT target schema

54530d7

Test ComfortViewFactorAngles translation

1c97302

Register ComfortViewFactorAngles translation

eb7fe45

Clean up angle factor name.

96adc29

Update ComfortViewFactorAngles schema

d536120

Refine ComfortViewFactorAngles model API

75bd4dd

Refine ComfortViewFactorAngles translation

cef6abb

Fix ComfortViewFactorAngles model integration

9a9d173

Add direct dependencies to ComfortViewFactorAngles translator tests

25b1e1a

Correct ComfortViewFactorAngles translation test paths and People fie…

85e090a

…ld indices

Update ComfortViewFactorAngles EnergyPlus IDD metadata

b0f4b5c

Require a name, declare the minimum field count, and document the enclosure requirement for referenced surfaces.

Require ComfortViewFactorAngles surfaces in one thermal zone

2a92c96

Validate surface space and thermal-zone assignments before adding angle factors, reject cross-zone lists, and align extensible-group update handling with ZoneMRTCalculation.

Reject cross-zone ComfortViewFactorAngles during translation

25785c8

Revalidate every referenced surface's thermal zone before generating the EnergyPlus object and cover stale surface assignments in the forward-translator tests.

Harden ComfortViewFactorAngles and MRT target validation

b857c7b

Reject non-finite angle factors and clear the MRT target when resetting the calculation type, with model regression coverage.

joseph-robertson added 2 commits September 10, 2026 10:45

Migrate legacy People MRT targets to definitions

24d1ad7

Move unambiguous 3.11 People MRT targets to PeopleDefinition during version translation, report shared-definition conflicts, and cover the migrated surface target.

Support all heat-transfer surfaces in ComfortViewFactorAngles

f940f2e

Accept Surface, SubSurface, and InternalMass targets from AllHeatTranSurfNames, preserve same-zone validation, and cover model and EnergyPlus translation behavior.

jmarrec force-pushed the comfort-view-factor-angles branch from 0b23e76 to f940f2e Compare September 10, 2026 08:45

jmarrec mentioned this pull request Sep 10, 2026

Wrap ComfortViewFactorAngles NatLabRockies/OpenStudio-resources#233

Open

26 tasks

Navigation Menu

Uh oh!

Wrap ComfortViewFactorAngles - #5650

Wrap ComfortViewFactorAngles#5650
joseph-robertson wants to merge 20 commits into
developfrom
comfort-view-factor-angles

joseph-robertson commented Sep 8, 2026 •

edited by jmarrec

Loading

Uh oh!

joseph-robertson commented Sep 8, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

joseph-robertson Sep 9, 2026

Uh oh!

joseph-robertson Sep 9, 2026

Uh oh!

Uh oh!

joseph-robertson Sep 9, 2026

Uh oh!

jmarrec commented Sep 9, 2026

Uh oh!

github-actions Bot commented Sep 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		if (result && istringEqual(mrtType, "EnclosureAveraged")) {
		resetSurfaceNameAngleFactorListName();

Uh oh!

Conversation

joseph-robertson commented Sep 8, 2026 • edited by jmarrec Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request overview

Pull Request Author

Review Checklist

Uh oh!

joseph-robertson commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

joseph-robertson Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

joseph-robertson Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

joseph-robertson Sep 9, 2026

Choose a reason for hiding this comment

Uh oh!

jmarrec commented Sep 9, 2026

Uh oh!

github-actions Bot commented Sep 10, 2026

🧪 Test Results Dashboard

Summary

❌ Significant Test Failures

📊 Test Run Information

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

joseph-robertson commented Sep 8, 2026 •

edited by jmarrec

Loading

joseph-robertson commented Sep 8, 2026 •

edited

Loading